Skip to content

Allow setting a map title when creating configuration files - #297

Merged
TheWitness merged 14 commits into
Cacti:developfrom
alcatron:feat/create-map-title
Oct 11, 2026
Merged

TheWitness merged 14 commits into
Cacti:developfrom
alcatron:feat/create-map-title

Conversation

@alcatron

@alcatron alcatron commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Add an optional Map Title field when creating configuration files. Normalize control characters to spaces, then apply the existing editor title sanitization before saving either a blank map or a copy.

An empty override retains a copied map's title and settings. A supplied title changes only the new copy. A blank source does not attempt to read the config directory. The title handling follows the editor's existing entity-encoding behavior rather than storing raw markup.

Validation: 226 PHP 8.3 tests passed in an isolated fixture layout. Real-engine save/reload tests cover punctuation, markup characters, line-break normalization, blank/copy paths, preserved source settings and an unchanged source file. Syntax, whitespace and manifest checks passed. The coverage gate reports no measured changed production lines because the management entry point has an existing exemption. Full running-installation integration remains unverified for this revision.

Latest review follow-up

Escaped the new translated map-title label and placeholder using __esc(). Added an actual form-fragment rendering regression with translated apostrophes and markup. Latest local validation: 227 PHP tests (5419 assertions). The management entry point is excluded from changed-line coverage; this regression executes the relevant form fragment directly.

These follow-up checks use isolated local fixtures, not a running Cacti installation. PHP 8.3 syntax, whitespace, manifest and translation-template checks passed. GitHub CI must still confirm the revised commits.

Follow-up 3c9c492 normalizes the optional title before checking whether to override an existing/default map title. Real-engine tests cover control-only and mixed-control input; the updated 227-test PHP suite passed locally (5419 assertions).

@TheWitness
TheWitness requested review from TheWitness and a balanced review from Copilot October 9, 2026 15:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Raw title metacharacters can produce invalid XML or markup injection in existing output sinks.

2 open findings
What changed in this PR

Adds optional map titles during configuration creation and copying.

Changes:

  • Passes normalized titles through the creation workflow.
  • Preserves copied settings unless a title override is supplied.
  • Adds engine regression tests, translations, and changelog documentation.
File Description
weathermap-cacti-plugin-mgmt.php Adds title UI and creation logic.
tests/​Unit/​NewMapTitleEngineTest.php Runs the engine regression test.
tests/​Support/​NewMapTitleEngineRegression.php Tests title persistence and map copying.
locales/​po/​cacti.pot Adds title-related translations.
CHANGELOG.md Documents the feature.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread weathermap-cacti-plugin-mgmt.php Outdated
Comment thread weathermap-cacti-plugin-mgmt.php Outdated
alcatron pushed a commit to alcatron/plugin_weathermap that referenced this pull request Oct 9, 2026
@TheWitness
TheWitness requested a balanced review from Copilot October 9, 2026 18:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new UI output needs escaping, and new fixture functions lack required PHPDoc.

1 open finding
2 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Low severity Add required PHPDoc for new fixture functions

tests/​Support/​NewMapTitleEngineRegression.php:32

Add the required PHPDoc for this new fixture function, matching the documented function shape used by the other support fixtures.

This issue also appears in the following locations of the same file:

  • line 38
  • line 46

🧠 Review effort: Balanced

Comment thread weathermap-cacti-plugin-mgmt.php Outdated
@alcatron
alcatron force-pushed the feat/create-map-title branch from fcf34c6 to e2d812d Compare October 9, 2026 23:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Control-only overrides can erase default or copied titles after normalization.

1 open finding
1 resolved since last review

🧠 Review effort: Balanced

Comment thread weathermap-cacti-plugin-mgmt.php Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The title workflow is correctly sanitized and covered across blank-map, copied-map, and escaping paths.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The implementation satisfies the stated behavior, addresses prior feedback, and passes current automated checks.

0 open findings

🧠 Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The implementation handles blank and copied maps correctly and includes focused regression coverage.

0 open findings

🧠 Review effort: Balanced

@alcatron

Copy link
Copy Markdown
Contributor Author

Merged latest develop and resolved the changelog/translation metadata conflicts in 305ef72, preserving the feature changes. All 232 local PHP tests pass (5466 assertions); syntax, manifest, whitespace and translation-template checks pass. These remain isolated fixture checks, not a fresh full-installation browser audit.

@alcatron

Copy link
Copy Markdown
Contributor Author

Resolved the new changelog conflict after #292 merged. All 235 local tests pass (5491 assertions), with syntax, manifest and whitespace checks passing. The feature changes remain intact; validation uses isolated fixtures.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Oversized titles can cross the config reader’s fixed line boundary and inject additional directives.

1 open finding

🧠 Review effort: Balanced

Comment thread weathermap-cacti-plugin-mgmt.php

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The implementation handles sanitization, copied-map semantics, parser boundaries, translations, and regression coverage without unresolved issues.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

@alcatron

Copy link
Copy Markdown
Contributor Author

Resolved the CHANGELOG.md conflict after #293 merged, preserving both entries. The feature code merged cleanly; all 236 local tests pass, plus manifest and whitespace checks. Validation uses isolated fixtures.

@alcatron

Copy link
Copy Markdown
Contributor Author

Updated again after #294 merged during the previous push. Resolved the changelog/translation metadata conflicts; code merged cleanly. All 262 local tests pass on this combined revision. Manifest and whitespace checks pass; validation uses isolated fixtures.

TheWitness
TheWitness previously approved these changes Oct 11, 2026
@alcatron

Copy link
Copy Markdown
Contributor Author

Resolved the new CHANGELOG.md conflict after #295 merged. The title and blank-map preset code merged cleanly. All 268 local tests pass (5615 assertions), including real-engine title/preset regressions. PHP syntax, manifest and whitespace checks pass; validation uses isolated fixtures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants